Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup load_program() in bank.rs #32146

Merged
merged 4 commits into from
Jul 21, 2023

Conversation

pgarg66
Copy link
Contributor

@pgarg66 pgarg66 commented Jun 14, 2023

Problem

The load_program function can use some streamlining and cleanup. The old code was written because of executor cache usage of bpf_loader. Since then the executor cache code has been deleted.

Summary of Changes

  • Rework load_program() to remove the call to load_program_from_account()
  • Remove load_program_from_account()

Fixes #

@pgarg66 pgarg66 force-pushed the cleanup-bank-load-program branch 2 times, most recently from d35b78b to 2de326c Compare June 15, 2023 14:59
@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #32146 (b4bc25a) into master (faa002c) will increase coverage by 0.0%.
The diff coverage is 91.0%.

@@           Coverage Diff           @@
##           master   #32146   +/-   ##
=======================================
  Coverage    82.0%    82.0%           
=======================================
  Files         780      780           
  Lines      210860   210843   -17     
=======================================
+ Hits       172944   172963   +19     
+ Misses      37916    37880   -36     

@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jun 30, 2023
@pgarg66 pgarg66 removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jul 5, 2023
@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jul 20, 2023
@pgarg66 pgarg66 removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jul 20, 2023
@pgarg66 pgarg66 marked this pull request as ready for review July 20, 2023 23:11
@pgarg66 pgarg66 requested a review from Lichtso July 20, 2023 23:11
Copy link
Contributor

@Lichtso Lichtso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we are missing a lot of test coverage for all the ways load_program() can go wrong.

}) = transaction_accounts[0].1.state()

if !solana_bpf_loader_program::check_loader_id(program_account.owner()) {
return ProgramAccountLoadResult::InvalidAccountData;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this originally returned InstructionError::IncorrectProgramId in load_program_from_account() and all errors from there were turned into LoadedProgramType::FailedVerification in load_program(). But it should be unreachable, so try placing a debug_assert(solana_bpf_loader_program::check_loader_id(); here instead.

runtime/src/bank.rs Show resolved Hide resolved
runtime/src/bank.rs Show resolved Hide resolved
runtime/src/bank.rs Show resolved Hide resolved
Lichtso
Lichtso previously approved these changes Jul 21, 2023
Copy link
Contributor

@Lichtso Lichtso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exact error codes between load_program_from_account() and load_program() might have changed but that is irrelevant since load_program() corrects all of them again and load_program_from_account() is not used anywhere else.

@pgarg66 pgarg66 merged commit fc35b13 into solana-labs:master Jul 21, 2023
4 checks passed
@pgarg66 pgarg66 deleted the cleanup-bank-load-program branch July 21, 2023 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants